refactor(core): simplify cross-stack reference detection #11254
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current design of cross-stack references is needlessly
complicated.
It is based around being able to hook into the Token resolution
mechanism, which is then used to mock-resolve the entire construct tree
and remember all the `CfnReference` objects that were encountered while
doing so.
Then, we poke additional state into each of the encountered tokens so
that *next time* that same token object is resolved in the scope of a
consuming stack, we can output one of the poked values.
This whole solution introduces the following complications:
- Scope-based resolution: instead of resolving to a simple value,
the token executes code that inspects the scope of the resolver
to return a different value.
- Time-based resolution: depending on when the token is being
resolved, it returns something different.
Both of these are the enemy of caching, and of consistency and
predictable behavior.
A better solution to always resolve to a consistent value that
represents the intended special semantics. In that way, the token itself
is trivial (all it needs to do is escape the type system and nothing
else), and template language postprocessors implement the special
features that are necessary.
For references, the issue is that the same token must *sometimes*
evaluate to a `{ Ref }` intrinsic and sometimes must evaluate to a
`{ Fn::ImportValue }` intrinsic.
In this PR, we extend the template language that Tokens render to
from CloudFormation to **CdkFormation**, which is a superset of
CloudFormation that also contains the intrinsic:
```
{ "$Cdk::Ref": ["/path/to/node", "Attr"] }
```
This makes references be able to render to a stable value again,
independent of scope and time.
Finally, in `stack.resolve()` the function `replaceReferences()`
is called which translates from CdkFormation -> CloudFormation,
replacing `{ $Cdk::Ref }` with `{ Ref }` or `{ Fn::ImportValue }`
as the case may be.
It has not yet been fully carried through in this PR, but this change
will be able to get rid of all the fields on `IResolveContext` and all
the resolution hook machinery that make the Token implementation
so complicated.
|
|
|
There competing PRs out there that introduce race conditions with this PR. Specifically, there is work to be done in the JSONinification PR. It should be clear that this PR is not production quality yet. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
eladb
approved these changes
Nov 2, 2020
| */ | ||
| public static forPseudo(pseudoName: string, scope: Construct) { | ||
| return CfnReference.singletonReference(scope, `Pseudo:${pseudoName}`, undefined, () => { | ||
| const cfnIntrinsic = { Ref: pseudoName }; | ||
| const cfnIntrinsic = { '$Cdk::Ref': [scope.node.path, pseudoName] }; |
There was a problem hiding this comment.
Define a const for "$CDK::Ref" and use it everywhere. Also add a comment explaining what is it
Suggested change
| const cfnIntrinsic = { '$Cdk::Ref': [scope.node.path, pseudoName] }; | |
| const cfnIntrinsic = { '$CDK::Ref': [scope.node.path, pseudoName] }; |
| if (typeof template === 'object' && template !== null) { | ||
| const keys = Object.keys(template); | ||
|
|
||
| if (keys.length === 1 && keys[0] === '$Cdk::Ref') { |
There was a problem hiding this comment.
Suggested change
| if (keys.length === 1 && keys[0] === '$Cdk::Ref') { | |
| if (keys.length === 1 && keys[0] === CDK_REF_KEY) { |
RomainMuller
force-pushed
the
master
branch
from
924c117 to
ebfd5f2
Compare
|
This PR has been deemed to be abandoned, and will be closed. Please create a new PR for these changes if you think this decision has been made in error. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current design of cross-stack references is needlessly
complicated.
It is based around being able to hook into the Token resolution
mechanism, which is then used to mock-resolve the entire construct tree
and remember all the
CfnReferenceobjects that were encountered whiledoing so.
Then, we poke additional state into each of the encountered tokens so
that next time that same token object is resolved in the scope of a
consuming stack, we can output one of the poked values.
This whole solution introduces the following complications:
the token executes code that inspects the scope of the resolver
to return a different value.
resolved, it returns something different.
Both of these are the enemy of caching, and of consistency and
predictable behavior.
A better solution to always resolve to a consistent value that
represents the intended special semantics. In that way, the token itself
is trivial (all it needs to do is escape the type system and nothing
else), and template language postprocessors implement the special
features that are necessary.
For references, the issue is that the same token must sometimes
evaluate to a
{ Ref }intrinsic and sometimes must evaluate to a{ Fn::ImportValue }intrinsic.In this PR, we extend the template language that Tokens render to
from CloudFormation to CdkFormation, which is a superset of
CloudFormation that also contains the intrinsic:
This makes references be able to render to a stable value again,
independent of scope and time.
Finally, in
stack.resolve()the functionreplaceReferences()is called which translates from CdkFormation -> CloudFormation,
replacing
{ $Cdk::Ref }with{ Ref }or{ Fn::ImportValue }as the case may be.
It has not yet been fully carried through in this PR, but this change
will be able to get rid of all the fields on
IResolveContextand allthe resolution hook machinery that make the Token implementation
so complicated.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license